There are two new features S3_TRANSFER_UPLOAD_DIRECTORY and S3_TRANSFER_DOWNLOAD_DIRECTORY we should add here

You can override the version in the subproject rather than hacking it in here

Or better, override / set hllPreviewVersion in the submodules which need it, and use sdkVersion as the default

I tried both options but it resulted in too much boilerplate in non sdkVersion modules, basically every module in hll. Decided to keep it like this for now.

Ok, instead of setting hllPreviewVersion in all hll modules, can you set sdkVersion in s3-transfer-manager? That should only need to be done in one place and removes this hack

You can use spaces in the display name, S3 Transfer Manager

Instead of depending on an already-published version of S3, we should require S3 to be bootstrapped locally for S3TM to build. Like how we did it for DDB Mapper

This was an intentional choice to help prevent changes to S3 from breaking the S3 TM. We don't need to have the latest APIs from S3.

I don't think it would happen often, but I think either approach is fine. The current one is just slightly safer.

Doesn't that mean that S3TM will always depend on the n-1 S3 client? So S3TM-1.2.3 uses S3-1.2.2? That doesn't seem right.

We want to catch changes to S3 which break S3TM and fix them as soon as possible. All of our modules are a comprehensive platform and should use the same, unified set of dependency versions.

These tests should either be made generic or not committed yet

Oh I see they have an @Ignore annotation, that seems fine too

suggestion: This and the function below might be better as extension functions

naming: add S3 / S3 Transfer Manager somewhere in the name for better IDE search discovery

correctness: the underlying client will emit a business metric for every request, whether the S3TM was used or not. Is that what we want?

withConfig makes a copy of the client with some overrides as far as I understand

Oh right

this feels better as a lateinit var

Let's throw a better Exception here and elsewhere

This isn't a scenario the user can cause themselves, is it? This could only happen because of a bug in our S3TM implementation it seems. If so, I'm generally fine with non-specific errors since we cannot expect users to catch them and act on them.

Correctness: The builder shouldn't be passing all of its values to the class's constructor. The constructor should accept a builder object and set its own fields that way. This results in less boilerplate:

public class UploadFileRequest(builder: UpdateFileRequest.Builder) {
    public val acl = builder.acl
    public val body = builder.body
    public val bucket = builder.bucket
    // ...etc...

    public companion object {
        public operator fun invoke(block: Builder.() -> Unit): UploadFileRequest =
            Builder().apply(block).build()
    }

    public class Builder {
        public var acl: ObjectCannedAcl? = null
        public var body: ByteStream? = null
        public var bucket: String? = null
        // ...etc...

        internal fun build(): UploadFileRequest = UploadFileRequest(this)
    }
}

We follow this pattern in our codegenned shapes and many of our handwritten ones.

Comment applies to other types in this PR.

Nit: Generally our build methods are internal because we don't want users to think they have to invoke it manually inside of a DSL block:

val req = UploadFileRequest {
    bucket = "foo"
    key = "bar"
    build() // <-- this is unnecessary
}

Question: These classes, their builders, their converters, etc. all seem like a lot of repeated boilerplate. Why aren't we code-generating these based on the list of fields from the spec?

Nit: This function doesn't feel like it belongs in this file. Nothing else in UploadFile.kt uses it, only S3TransferManager.kt.

Nit: 10000 is already a constant in this file. Reuse that constant in this string rather than duplicating it. This'll reduce the likelihood that the values get out of sync.

Style: This class has too much logic inside of it and that won't be sustainable once we add more operations and variants. We should refactor this, possibly so that individual operations are located in a single file and maybe even represented by a class.

Style: Method is too long and should be refactored.

Correctness: This method shouldn't return Deferred<UploadFileResponse>, it should return UploadFileResponse just like our low-level APIs do. If users want this to happen asynchronously, they can wrap it in async { } themselves.

Note that coroutineScope does not return until all the work inside is completed (including child coroutines) so the Deferred value would already be filled anyway.

Question: Why do all of these hooks have the same TransferContext parameter? Our other interceptors have unique context parameters per hook because we gradually accumulate more context over the lifetime of the request. For instance, readBeforeFileTransferred can never have an upload ID but readAfterFileTransferred can.

I made the items in the context nullable so we wouldn't have to create different context types. It's less complex, and interceptors can always just use ?..

Correctness: We should not have JVM-only tests in most cases. S3TM must work across all targets and our test dependencies should be available on all targets.

naming: smithy-kotlin-http-test-jvm

what does this TODO mean?

This was a note to myself, I forgot to delete it.

Have we decided on a package name yet? I think s3tm would be simpler, cleaner

I can go either way, not sure if Ian has an opinion.

naming: IoMapping? same applies in other places you use IO

FYI this doesn't work for native, I have a fix in my kn-merge-main branch. You'll probably need to refactor to take that change once I merge it

How did we decide this number?

With our 8MB byte default part size that means up to 40MB could be used as a buffer. Given we don't know anything about the user's env and memory limitations, 40MB seems like not too much but not too little.

naming: having the name "implementation" in a function implementation feels wrong

We do it all over the SDK and Smithy Kotlin, but we usually use the suffix impl.

Did we clarify with the spec author what level this should be logged at?

Let me ask

There's nothing in the spec mentioning logging a message when the configured part size isn't used btw.

The spec author uses DEBUG